Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Preserve handler function attributes across middlewares #4195

Merged
merged 1 commit into from
Oct 26, 2019
Merged

Preserve handler function attributes across middlewares #4195

merged 1 commit into from
Oct 26, 2019

Conversation

gjcarneiro
Copy link
Contributor

@gjcarneiro gjcarneiro commented Oct 16, 2019 •
edited
Loading

What do these changes do?

When using middlewares (such as the hidden middleware implicitly added by subapps), it fixes the functools.partial() usage on the handler view, to copy any attributes from the view function.

Are there changes in behavior for the user?

Now any attributes added to a view handler function, e.g. by a decorator, are seen by all the middlewares in the chain.

Before this change, attributes we only seen by a middleware in the case of the last middleware (first to run), and only for the main Application, for subapps that wasn't true.

Related issue number

#4174

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 16, 2019
@gjcarneiro gjcarneiro changed the title Preserve handler function attributes for the case of subapps WIP: Preserve handler function attributes for the case of subapps Oct 16, 2019
@gjcarneiro gjcarneiro changed the title WIP: Preserve handler function attributes for the case of subapps Preserve handler function attributes for the case of subapps Oct 16, 2019
@codecov-io
Copy link

codecov-io commented Oct 16, 2019 •
edited
Loading

Codecov Report

Merging #4195 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4195      +/-   ##
==========================================
- Coverage   97.57%   97.56%   -0.02%     
==========================================
  Files          43       43              
  Lines        8825     8825              
  Branches     1381     1381              
==========================================
- Hits         8611     8610       -1     
- Misses         92       93       +1     
  Partials      122      122
Impacted Files Coverage Δ
aiohttp/web_app.py 97.3% <100%> (ø) ⬆️
aiohttp/web_fileresponse.py 97.72% <0%> (-0.57%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6236536...391c592. Read the comment docs.

@gjcarneiro
Copy link
Contributor Author

I modified a unit test to check for this.

I don't know whether this behaviour should be documented explicitly. Or else developers will find that it just works, and needs no documentation as such.

@gjcarneiro gjcarneiro changed the title Preserve handler function attributes for the case of subapps Preserve handler function attributes across middlewares Oct 16, 2019
asvetlov
asvetlov requested changes Oct 18, 2019
View reviewed changes
Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated test is for application+middleware.

Please provide a test that checks sub-app handler attributes in parent-app middleware.

@gjcarneiro
Copy link
Contributor Author

Pushed a new test_middleware_subapp test.

asvetlov
asvetlov approved these changes Oct 25, 2019
View reviewed changes
Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
I'll merge it soon; when will have time to generate backport PR as well

@gjcarneiro
Copy link
Contributor Author

Thanks! Well, for me, personally, there is no need to backport it, necessarily. I have an ugly workaround in the code, it can wait a few months.

@asvetlov asvetlov merged commit 2c70eb8 into aio-libs:master Oct 26, 2019
@asvetlov
Copy link
Member

Thanks.
The PR can be backported easily, there is no reason to postpone the feature delivery.

asvetlov pushed a commit that referenced this pull request Oct 26, 2019
@gjcarneiro @asvetlov
[3.7] Preserve view handler function attributes across middlewares (#…
2f424e6 
…4195).

(cherry picked from commit 2c70eb8)

Co-authored-by: Gustavo J. A. M. Carneiro <gjcarneiro@gmail.com>
asvetlov added a commit that referenced this pull request Oct 26, 2019
@asvetlov @gjcarneiro
[3.7] Preserve view handler function attributes across middlewares (#…
5f15721 
…4195). (#4276)

(cherry picked from commit 2c70eb8)

Co-authored-by: Gustavo J. A. M. Carneiro <gjcarneiro@gmail.com>
asvetlov pushed a commit that referenced this pull request Oct 16, 2020
@gjcarneiro @asvetlov
Preserve view handler function attributes across middlewares (#4195)
fb35b25
@bdraco bdraco mentioned this pull request Sep 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asvetlov asvetlov asvetlov approved these changes

Assignees
No one assigned
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gjcarneiro @codecov-io @asvetlov